Skip to content

Rescue errors in Supervisor server. #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 16, 2025
Merged

Rescue errors in Supervisor server. #3

merged 1 commit into from
Oct 16, 2025

Conversation

@macournoyer
Copy link
Contributor

@macournoyer macournoyer commented Oct 16, 2025 •
edited
Loading

Supervisor shouldn't fail on exceptions. Just like a production web app return 500, but continues accepting connections.

macournoyer
macournoyer commented Oct 16, 2025
View reviewed changes
lib/async/container/supervisor/server.rb
connection = Connection.new(peer, 1)
connection.run(self)
rescue => error
Console.error(self, "Error while accepting connection!", exception: error)
Copy link
Contributor Author

@macournoyer macournoyer Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is the most important, or else any error will silently stop the Fiber here: https://github.com/socketry/io-endpoint/blob/main/lib/io/endpoint/wrapper.rb#L169, and potentially exit the accept loop.

@samuel-williams-shopify samuel-williams-shopify merged commit 19d97d5 into socketry:main Oct 16, 2025
16 of 20 checks passed
@samuel-williams-shopify
Copy link
Contributor

samuel-williams-shopify commented Oct 16, 2025

LGTM.

@samuel-williams-shopify
Copy link
Contributor

samuel-williams-shopify commented Oct 17, 2025

I ended up reverting this because I don't think it's necessary and the error handling is done at the dispatcher level is correct (and this change can hide errors).

  1. As separately discussed, the supervisor can die and it should be restarted - if that's not happening, it's a different issue.
  2. endpoint.accept do ... end runs each connection in a separate task and it's okay for them to fail. It won't break the accept loop.
  3. ::Process.kill(signal, ::Process.ppid) should never fail if the current process is alive. Since it's sending a signal to it's own process group. However, if for some reason it fails, only the call will fail.
  4. Monitor do_status can fail and that's okay. Skipping failures might hide issues. Better to fail.

I added tests for most of the cases above in be9f242

Thanks for raising this issue and working on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@macournoyer @samuel-williams-shopify